test: harden reliable test execution#705
Conversation
…y-main-integration # Conflicts: # .github/workflows/ci.yml # docs/branch-review-ledger.md # package.json # playwright.config.ts # scripts/ci-change-scope.mjs # scripts/verify-pr-local.mjs
…y-main-integration
…y-main-integration # Conflicts: # docs/branch-review-ledger.md
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe PR restructures CI and local verification, adds shared heavy-run and offline test safeguards, isolates Playwright production runs, tightens flake classification, and updates UI tests to use deterministic readiness and event synchronization. ChangesCI and test reliability hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3581b10040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3581b10040
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
tests/ui-smoke.spec.ts (1)
1685-1686: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetain deterministic keyboard-focus coverage for the privacy link.
Removing these assertions means the flow can pass even if keyboard focus is lost before navigation. Replace the flaky sequence with a stable
toBeFocused()assertion rather than dropping the accessibility check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/ui-smoke.spec.ts` around lines 1685 - 1686, Restore deterministic keyboard-focus coverage in the privacy-link flow around page.goto by asserting the privacy link is focused with toBeFocused() before navigation. Replace the flaky focus assertion sequence without removing the accessibility check, and retain the existing navigation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/process-hardening.md`:
- Around line 141-142: Update the exhaustive dependency list in the CI aggregate
description to include the RAG fixture validation workflow job, using its exact
job ID from the workflow and preserving the existing scope-enforcement wording.
In `@docs/testing.md`:
- Line 5: Update the lock-recovery guarantee in the testing documentation to
state that ownerless locks are also reclaimed after the initialization grace
period, while retaining the existing behavior for locks whose recorded process
is demonstrably dead. Ensure the wording accurately reflects the recovery logic
implemented by test-run-lock.mjs.
In `@scripts/ci-change-scope.mjs`:
- Line 157: Update the coveragePatterns regular expression near the existing
run-* script entries to include scripts/run-playwright.mjs, preserving the
current matching behavior for all other scripts so the coverage self-test
recognizes this runner as coverage-related.
In `@scripts/classify-playwright-failures.mjs`:
- Around line 24-28: Update failedTestcasesFromJunit so failure detection
removes CDATA sections and XML comments from each testcase body before testing
for failure or error elements. Preserve detection of actual JUnit failure/error
nodes, and add a fixture covering a passing testcase whose system-out contains a
CDATA-wrapped failure string.
In `@scripts/eval-rag-offline.mjs`:
- Around line 5-9: Update the child-process invocation in the script loop to run
from the repository root rather than inheriting the caller’s working directory.
Derive the root from the launcher script’s location and pass it as the spawnSync
working-directory option, preserving the existing script paths and argument
forwarding.
In `@scripts/flake-ledger.mjs`:
- Around line 22-25: Update dateValue to round-trip the parsed UTC date into
YYYY-MM-DD and compare it with the original value before accepting it; throw the
existing invalid-date error when they differ, so calendar-invalid values such as
2026-02-30 are rejected. Add a regression test covering this case.
In `@scripts/playwright-base-url.ts`:
- Around line 94-100: Update the URL resolution flow around
findExistingLocalProjectUrl and allowEnsure so the !allowEnsure guard runs
before any existing-server discovery, requiring explicit PLAYWRIGHT_BASE_URL in
that mode. Only perform existing-server fallback or server ensuring when
allowEnsure is true.
In `@scripts/run-playwright.mjs`:
- Around line 94-99: Update findFreePort to scan the configured port range
circularly, starting at startPort and wrapping from projectPortEnd back to the
range’s lower bound. Preserve the existing isReservedDevPort and canListen
checks, and only throw after every configured port has been examined.
- Around line 38-42: Update the mockup-project detection in the playwright
argument parsing flow so mockups are enabled when no --project filter is
supplied, since Playwright then runs all projects. Preserve the existing
detection for explicit --project=chromium-mockups and --project chromium-mockups
arguments, and ensure the corresponding mockup route/build configuration uses
the resulting flag.
In `@tests/flake-ledger.test.ts`:
- Around line 60-66: Update the expiry-boundary test in
validateFlakeLedgerEntries to capture a single current time before constructing
the expiry date and validating the entry, then derive isoDate(31) from that
captured clock. Ensure both date generation and validation use the same
timestamp so the 31-day rejection remains deterministic, while preserving the
existing isoDate(30) acceptance assertion.
In `@tests/property-seed.ts`:
- Around line 4-6: Update the parsedSeed validation in tests/property-seed.ts to
validate the raw FAST_CHECK_SEED text before calling Number.parseInt, rejecting
malformed values such as 123junk, 1.9, and 1e3 while preserving the
defaultPropertySeed fallback and safe-integer requirement.
In `@tests/universal-search.test.ts`:
- Around line 3-15: Update the test teardown in afterEach to unmock every module
specifier registered by vi.doMock in this test file, using matching vi.doUnmock
calls or a shared cleanup helper. Keep the existing module-reset behavior and
ensure mocked factories cannot affect later tests.
In `@tests/worker-bundle.test.ts`:
- Line 23: Update the child-process invocation within the test “keeps every
external import resolvable under plain node with prod-only deps” to enforce a
subprocess-level timeout, or replace the blocking execFileSync call with an
awaited child-process API that supports timeout handling. Preserve the existing
60-second test timeout and probe behavior.
---
Nitpick comments:
In `@tests/ui-smoke.spec.ts`:
- Around line 1685-1686: Restore deterministic keyboard-focus coverage in the
privacy-link flow around page.goto by asserting the privacy link is focused with
toBeFocused() before navigation. Replace the flaky focus assertion sequence
without removing the accessibility check, and retain the existing navigation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4b937ce-e6b4-4100-b9ef-996111d175c9
📒 Files selected for processing (60)
.github/workflows/ci-triage.yml.github/workflows/ci.yml.gitignoreAGENTS.mdREADME.mddocs/branch-review-ledger.mddocs/codebase-index.mddocs/process-hardening.mddocs/testing.mdnext.config.tspackage.jsonplaywright.config.tsplaywright.visual.config.tsscripts/check-format-changed.mjsscripts/check-rag-fixtures.mjsscripts/child-process-result.mjsscripts/ci-change-scope.mjsscripts/classify-playwright-failures.mjsscripts/eval-rag-offline.mjsscripts/flake-ledger.mjsscripts/playwright-base-url.tsscripts/run-heavy.mjsscripts/run-live-tests.mjsscripts/run-playwright.mjsscripts/run-vitest.mjsscripts/test-environment.mjsscripts/test-focused.mjsscripts/test-rag-offline.mjsscripts/test-run-lock.mjsscripts/verify-pr-local.mjssrc/instrumentation.tssrc/proxy.tstests/architecture-boundaries.test.tstests/ci-cache-safety.test.tstests/client-secret-surface.test.tstests/demo-data.test.tstests/flake-ledger.jsontests/flake-ledger.test.tstests/instrumentation.test.tstests/property-accessible-table.test.tstests/property-chunking.test.tstests/property-numeric-token-preservation.test.tstests/property-seed.tstests/proxy.test.tstests/rag-answer-fallback.test.tstests/test-runner-safety.test.tstests/tsx-server-only-runner.test.tstests/ui-accessibility.spec.tstests/ui-formulation.spec.tstests/ui-smoke.spec.tstests/ui-stress.spec.tstests/ui-tools-collapse.spec.tstests/ui-tools-task-directory.spec.tstests/ui-tools.spec.tstests/ui-visual-artifacts.spec.tstests/universal-search-owner.live.test.tstests/universal-search.test.tstests/verify-pr-local.test.tstests/worker-bundle.test.tsvitest.config.mts
…y-main-integration # Conflicts: # docs/branch-review-ledger.md
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Heuristic only — a main-side or flake label is a starting point, not a verdict. |
…y-main-integration # Conflicts: # tests/ui-smoke.spec.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/test-run-lock.mjs (1)
78-147: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftMake stale-lock recovery atomic before retrying.
The stale check and recursive removal are separate operations. Two contenders can both classify the lock as stale; one can acquire a replacement lock, then the other can delete that live lock and acquire its own. This breaks heavyweight-command serialization and can cause concurrent test/build runs.
Use an atomic compare-and-recover protocol or a proven lock primitive, and add a regression test with two simultaneous stale-lock recoverers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test-run-lock.mjs` around lines 78 - 147, Make stale-lock recovery in acquireHeavyRunLock atomic so a contender cannot delete a replacement lock created by another process after the stale check. Replace the separate readOwner/lockIsOldEnoughToRecover and rmSync sequence with an atomic compare-and-recover or proven lock primitive that verifies the same stale lock before removal, then retries acquisition safely. Add a regression test exercising two simultaneous stale-lock recoverers and assert only one acquires the lock while the other cannot invalidate it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/test-run-lock.mjs`:
- Around line 78-147: Make stale-lock recovery in acquireHeavyRunLock atomic so
a contender cannot delete a replacement lock created by another process after
the stale check. Replace the separate readOwner/lockIsOldEnoughToRecover and
rmSync sequence with an atomic compare-and-recover or proven lock primitive that
verifies the same stale lock before removal, then retries acquisition safely.
Add a regression test exercising two simultaneous stale-lock recoverers and
assert only one acquires the lock while the other cannot invalidate it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f750b6c3-9710-47e0-a9c0-7ee997bdce8e
📒 Files selected for processing (11)
docs/branch-review-ledger.mddocs/codebase-index.mdnext.config.tspackage.jsonplaywright.config.tsscripts/ci-change-scope.mjsscripts/run-playwright.mjsscripts/test-run-lock.mjssrc/proxy.tstests/test-runner-safety.test.tstests/ui-smoke.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- next.config.ts
- docs/codebase-index.md
- playwright.config.ts
- src/proxy.ts
- tests/test-runner-safety.test.ts
- package.json
- docs/branch-review-ledger.md
- scripts/run-playwright.mjs
- tests/ui-smoke.spec.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76f3dcf877
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/local-server-utils.test.ts (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new validation branches.
The test exercises only valid input, while
circularProjectPortRangenow explicitly rejects non-integer and out-of-range ports. Add focused cases for those fail-closed paths.Suggested coverage
it("scans the full port range circularly from the preferred port", () => { const ports = circularProjectPortRange(projectPortEnd); expect(ports.slice(0, 3)).toEqual([projectPortEnd, projectPortStart, projectPortStart + 1]); expect(new Set(ports).size).toBe(projectPortEnd - projectPortStart + 1); expect(ports.at(-1)).toBe(projectPortEnd - 1); }); + + it.each([projectPortStart - 1, projectPortEnd + 1, 3100.5, NaN])( + "rejects invalid start port %p", + (startPort) => { + expect(() => circularProjectPortRange(startPort)).toThrow( + /Project port must be between/, + ); + }, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/local-server-utils.test.ts` around lines 42 - 48, Add focused tests for circularProjectPortRange covering non-integer and out-of-range preferred ports, asserting each invalid input is rejected according to the function’s validation behavior. Keep the existing valid circular-order and full-range assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/local-server-utils.test.ts`:
- Around line 42-48: Add focused tests for circularProjectPortRange covering
non-integer and out-of-range preferred ports, asserting each invalid input is
rejected according to the function’s validation behavior. Keep the existing
valid circular-order and full-range assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53304fa3-0f16-4c5d-80db-2ea1bcddc6ef
📒 Files selected for processing (16)
docs/process-hardening.mddocs/testing.mdpackage.jsonscripts/classify-playwright-failures.mjsscripts/eval-rag-offline.mjsscripts/flake-ledger.mjsscripts/playwright-base-url.tsscripts/run-playwright.mjssrc/lib/local-server-utils.mjstests/flake-ledger.test.tstests/local-server-utils.test.tstests/property-seed-validation.test.tstests/property-seed.tstests/test-runner-safety.test.tstests/universal-search.test.tstests/worker-bundle.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/property-seed.ts
- scripts/eval-rag-offline.mjs
- docs/testing.md
- package.json
- tests/test-runner-safety.test.ts
- tests/flake-ledger.test.ts
- scripts/run-playwright.mjs
- scripts/flake-ledger.mjs
- scripts/classify-playwright-failures.mjs
- tests/universal-search.test.ts
- docs/process-hardening.md
Summary
Why
Concurrent heavyweight commands, provider exposure, repeated full suites, shared Playwright state, stale flake matching, and retrying browser actions were causing long runtimes, churn, and unreliable results.
Areas touched
Test runners and locks, Vitest/Playwright configuration, CI workflows and scope selection, flake ledger/classification, property/RAG/architecture test setup, browser regression specs, and contributor documentation.
Verification
npm run test -- tests/ci-cache-safety.test.ts tests/worker-bundle.test.ts— 4/4 passednpm run check:github-actions— passednpm run check:ci-scope— passednpm run check:function-grants— passednpm run verify:pr-local— passed: format, lint, typecheck, 2,518 unit tests, production build/client-secret scan, and 36 offline RAG fixturesnpm run verify:ui— passed: 212/212 required production Chromium journeys with zero retriesgit diff --check— passedChecks not run
test:live, OpenAI/Supabase provider-backed checks, release/provider evaluations, and deployments were not run because they require separate approval.npm run ensurewas attempted, but Turbopack rejects this integration worktree's temporary externalnode_modulesjunction. The owned Webpack production runner verified/api/local-project-idand completed the full Chromium gate instead.verify:cheapwas active. PR Refine header and footer scroll timing #703's own focused checks were green, the merge conflict was append-only documentation, and this PR's exact-head hosted checks remain required.Risk
No application API, database, dependency, migration, or production behavior is intentionally changed. Provider credentials are scrubbed only inside ordinary test processes. The main risk is CI workflow/routing behavior, covered by action-pin, CI-scope, runner-safety, and exact-head hosted checks.
Summary by CodeRabbit
/mockupsaccess to only specific offline/advisory configurations.